[RF] Zero out negative bins in RooHistPdf for consistent normalization - #23103
Merged
guitargeek merged 1 commit intoAug 21, 2026
Merged
Conversation
guitargeek
force-pushed
the
histpdf-clamp-negative-bins
branch
from
August 19, 2026 10:07
ae72e31 to
d2d9854
Compare
dpiparo
reviewed
Aug 19, 2026
guitargeek
force-pushed
the
histpdf-clamp-negative-bins
branch
from
August 19, 2026 11:57
d2d9854 to
773da6b
Compare
Test Results 22 files 22 suites 3d 12h 32m 58s ⏱️ For more details on these failures, see this check. Results for commit 3442985. ♻️ This comment has been updated with latest results. |
hageboeck
reviewed
Aug 21, 2026
hageboeck
approved these changes
Aug 21, 2026
hageboeck
left a comment
hageboeck
left a comment
Member
There was a problem hiding this comment.
Except for the implementation details that leaked into the comments, LGTM
dpiparo
approved these changes
Aug 21, 2026
dpiparo
left a comment
dpiparo
left a comment
Member
There was a problem hiding this comment.
LGTM once Stephan's comments are addressed.
guitargeek
force-pushed
the
histpdf-clamp-negative-bins
branch
from
August 21, 2026 13:49
773da6b to
0546b53
Compare
Contributor
Author
|
Thanks for the reviews! PR is updated with improved docs and error messages that don't talk about implementation details ("internal histogram clones") anymore. |
A RooHistPdf clamps negative bin contents to zero when evaluating, but its normalization integral was based on the plain sum of the bin weights, including the negative ones. A histogram with negative bins therefore yielded a pdf that did not integrate to unity, silently biasing in particular the yields of extended fits: even an Asimov fit with a model identical to the generating pdf did not close. Now, if the input histogram contains bins with negative content, these are set to zero in an internally-owned clone of the histogram that is used instead, with a warning. This makes the pdf value and its normalization consistent, restoring exact closure. The input histogram is not modified, and bin errors are preserved. The RooHistPdf constructed by RooAbsCachedPdf implementations like RooFFTConvPdf is unaffected, because the cache histogram is still empty at construction time and only filled (in place) afterwards. Fixes ROOT-10825. 🤖 Done with the help of AI
guitargeek
force-pushed
the
histpdf-clamp-negative-bins
branch
from
August 21, 2026 15:28
0546b53 to
3442985
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A RooHistPdf clamps negative bin contents to zero when evaluating, but its normalization integral was based on the plain sum of the bin weights, including the negative ones. A histogram with negative bins therefore yielded a pdf that did not integrate to unity, silently biasing in particular the yields of extended fits: even an Asimov fit with a model identical to the generating pdf did not close.
Now, if the input histogram contains bins with negative content, these are set to zero in an internally-owned clone of the histogram that is used instead, with a warning. This makes the pdf value and its normalization consistent, restoring exact closure. The input histogram is not modified, and bin errors are preserved.
The RooHistPdf constructed by RooAbsCachedPdf implementations like RooFFTConvPdf is unaffected, because the cache histogram is still empty at construction time and only filled (in place) afterwards.
Fixes ROOT-10825.
🤖 Done with the help of AI